Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Run build if formatting succeeds #822

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

sampathweb
Copy link
Collaborator

No description provided.

runs-on: ubuntu-latest
needs: [format]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also have nightly testing, which is in a different file (probably doesn't need to be, just the templated we copied). Can we do the same thing there?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nightly doesn't have the Code Format / Lint check. Should we add it there as well?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine if we consolidate all these to one file, but I don't think we would want to add the format/lint check twice.

There will be no difference for it nightly vs stable, and confusing to contributors to have it show up twice in the UI.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So basically, let's figure out a way to block the nightly test on the format check to, and maybe consolidate nightly and stable into one yaml if that helps.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since Nightly file was similar to actions, I added nightly schedule to actions and deleted nightly yaml. Is this what you were looking for?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah not quite. Nightly is not about the schedule so much as what it tests against. It is the only coverage we have again the nightly tensorflow/keras builds, so it's important mainly to catch places we break against tensorflow before the stable tf releases.

image

We don't want to lose that coverage. Nor do we want to only block our stable coverage on formatting (then we are only gating 1/3 or our overall testing on the format check, what's the point?).

I don't all the details on github actions, but potentially the thing to do is to consolidate to one file (maybe let's call this testing.yml instead of actions or nightly), make sure to keep the nightly workflow installing from requirements-nightly.txt, and have all workflows gate on the format check.

For triggers we might want the full gamut for all testing, e.g.

on:
  # Test every stable branch commit.
  push:
  # Test every pull request.
  pull_request:
  # Test once at midnight.
  schedule:
    - cron: '0 0 * * *'
  # Test on every tagged release.
  release:
    types: [created]

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a single file, I believe you can only have one "on" block. So, the jobs listed will all be run for that "on" block. So if we want to run some at Nightly and some on every PR, we may need the two files.
https://stackoverflow.com/questions/70093399/can-i-have-multiple-on-in-single-gitactions-workflow-file

Copy link
Member

@mattdangerw mattdangerw Mar 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can have the same on block for everything! The fact that our nightly testing runs every 24 and our stable testing runs every release (and not vice versa) is just our own bad maintenance, no grand plan there.

We want separate tests against tf-nightly and tensorflow. These can run (and should run) with exactly the same triggers. Running them every 24 hours probably isn't too important, but would help catch failures that develop over the weekend.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to be user friendly here, we could probably make the names a little clearer in the PR UI.

Tests / Check the code format
Tests / Run tests with TF Stable
Tests / Run tests with TF Nightly

@abheesht17
Copy link
Collaborator

One additional comment - to abandon previous commit tests when a new commit is pushed, we can do something like this: https://github.com/TeamAmaze/AmazeFileManager/blob/release/3.7/.github/workflows/android-feature.yml#L10-L12

@mattdangerw
Copy link
Member

One additional comment - to abandon previous commit tests when a new commit is pushed, we can do something like this: https://github.com/TeamAmaze/AmazeFileManager/blob/release/3.7/.github/workflows/android-feature.yml#L10-L12

Nice! I am down to try this out.

Note that we have a few trigger conditions, and we do probably want to make sure we run CI for every master branch commit. But given that we squash PRs, only running on the latest commit seems like a good move!

@sampathweb
Copy link
Collaborator Author

One additional comment - to abandon previous commit tests when a new commit is pushed, we can do something like this: https://github.com/TeamAmaze/AmazeFileManager/blob/release/3.7/.github/workflows/android-feature.yml#L10-L12

Added to actions.yml

@mattdangerw
Copy link
Member

Looks like this still needs some of the changes discussed above, but ping whenever this is ready for review!

@mattdangerw mattdangerw self-assigned this Mar 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants